Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add compatibility with individual date extensions for submissions #2026

Closed

Conversation

Ian2012
Copy link

@Ian2012 Ian2012 commented Aug 10, 2023

TL;DR - Adds compatibility with individual date extensions behind a waffle flag

Partially fixes: #1959

What changed?

  • If the due date of the submission is greater than the submission_due date, due is used for the submission_range.
  • If the due date of the assessment step is greater than the assessment_due date, due is used for the assesment_step range.

Developer Checklist

Testing Instructions

Add a waffle flag for everyone: openresponseassessment.due_date_extension

  1. Go to the CMS
  2. Add a default ORA component with any assessment steps.
  3. Set a due date for the Section to yesterday.
  4. Verify user can't answer the ORA.
  5. Go to the instructor tab, extensions, and grant an individual due date for that section.
  6. Verify user can answer the problem.

Before this change, users were unable to answer:

image

After setting a due date for yesterday:

image

After granting an individual date extension:

image

The second commit allows us to apply the individual due date extension for every step in the workflow:

c005190

image

image

Things to consider

Unit tests will be added once we receive our first product review and concerns are resolved

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

  • I've done a visual code review
  • I've tested the new functionality

FYI: @openedx/content-aurora

@Ian2012 Ian2012 requested a review from a team as a code owner August 10, 2023 13:01
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 10, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 10, 2023

Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@Ian2012 Ian2012 force-pushed the cag/individual-date-extensions branch 2 times, most recently from a483dc3 to 48d168e Compare August 10, 2023 13:38
openassessment/xblock/openassessmentblock.py Outdated Show resolved Hide resolved
openassessment/xblock/openassessmentblock.py Outdated Show resolved Hide resolved
openassessment/xblock/openassessmentblock.py Outdated Show resolved Hide resolved
@Ian2012 Ian2012 force-pushed the cag/individual-date-extensions branch from fe1cbe9 to f9e64b8 Compare August 11, 2023 18:40
@Ian2012 Ian2012 changed the title [WIP] feat: add compatibility with individual date extensions for submissions feat: add compatibility with individual date extensions for submissions Aug 11, 2023
@Ian2012 Ian2012 force-pushed the cag/individual-date-extensions branch 5 times, most recently from 36f655a to 1dce338 Compare August 11, 2023 20:27
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage: 64.06% and project coverage change: -0.12% ⚠️

Comparison is base (99bc938) 95.09% compared to head (bed275c) 94.98%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2026      +/-   ##
==========================================
- Coverage   95.09%   94.98%   -0.12%     
==========================================
  Files         158      158              
  Lines       17420    17482      +62     
  Branches     1622     1630       +8     
==========================================
+ Hits        16566    16605      +39     
- Misses        641      662      +21     
- Partials      213      215       +2     
Flag Coverage Δ
unittests 94.98% <64.06%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
openassessment/xblock/openassessmentblock.py 86.27% <28.12%> (-4.13%) ⬇️
openassessment/xblock/config_mixin.py 93.44% <100.00%> (+0.46%) ⬆️
openassessment/xblock/test/test_grade.py 99.10% <100.00%> (+0.01%) ⬆️
openassessment/xblock/test/test_openassessment.py 100.00% <100.00%> (ø)
...ssment/xblock/test/test_save_files_descriptions.py 100.00% <100.00%> (ø)
openassessment/xblock/test/test_save_response.py 100.00% <100.00%> (ø)
openassessment/xblock/test/test_self.py 100.00% <100.00%> (ø)
openassessment/xblock/test/test_staff_area.py 99.85% <100.00%> (+<0.01%) ⬆️
...penassessment/xblock/test/test_student_training.py 93.44% <100.00%> (+0.14%) ⬆️
openassessment/xblock/test/test_submission.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few more comments!

openassessment/xblock/openassessmentblock.py Outdated Show resolved Hide resolved
openassessment/xblock/openassessmentblock.py Outdated Show resolved Hide resolved
@Ian2012 Ian2012 force-pushed the cag/individual-date-extensions branch from 1dce338 to bed275c Compare August 11, 2023 21:26
@Ian2012
Copy link
Author

Ian2012 commented Aug 11, 2023

This PR is our first proposal to add this functionality, but we have only an edge case in which we would like your opinion:

If the Subsection due date is configured to any date after the submission_due, the subsection due date is used instead. But we don't think this should be the case.

One solution we have in mind is to query for a UserDate (which stores individual date extensions) object related to the subsection and to the student.

What do you think? @Colin-Fredericks @Daniel-hershel

@Colin-Fredericks
Copy link

If the Subsection due date is configured to any date after the submission_due, the subsection due date is used instead. But we don't think this should be the case.

One solution we have in mind is to query for a UserDate (which stores individual date extensions) object related to the subsection and to the student.

@Ian2012 So we've got 5 dates at work here:

  1. Subsection due date
  2. ORA submission due date
  3. ORA peer-grading due date
  4. Learner extensions to the submission date
  5. Learner extensions to the peer-grading date

My feeling is that learner extensions should override everything else, and the ORA dates should override the subsection due date. Does that make sense? Did I answer the wrong question?

@itsjeyd itsjeyd added the product review PR requires product review before merging label Aug 15, 2023
@itsjeyd
Copy link

itsjeyd commented Aug 15, 2023

@Ian2012 Thanks for these changes!

Hi @jmakowski1123, could you please create a feature ticket for this PR? Or will we not need one since there is an existing issue (#1959) that is being (partially) fixed here?

CC @e0d @mphilbrick211

@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Aug 15, 2023
@Daniel-hershel
Copy link

@Ian2012 I just wanted to give you a heads up that we are planning to launch a very similar feature in the next week or so. I am going to meet with the lead engineer working on that project later today to review how this PR compares to the feature implementation we are planning to launch, and we will give an update after that meeting. Thank you for your contribution.

@Ian2012
Copy link
Author

Ian2012 commented Aug 15, 2023

@Daniel-hershel thanks for the information

@Colin-Fredericks yes, that's the question. I will update the implementation to match this without tests, and wait for the result of your internal discussion

@e0d
Copy link

e0d commented Aug 15, 2023

@Ian2012 Thanks for these changes!

Hi @jmakowski1123, could you please create a feature ticket for this PR? Or will we not need one since there is an existing issue (#1959) that is being (partially) fixed here?

CC @e0d @mphilbrick211

We shouldn't need one. If #1959 doesn't meet the full need, we should update it.

@Daniel-hershel
Copy link

@Ian2012 I have met with our lead engineer on this project @jansenk , and my understanding is that the feature update we're about to launch will include the functionality included in this PR. The work we are finishing up will allow course authors to make a selection in studio that has 3 choices:

  • align ORA due dates with subsection due date
  • align ORA due dates with end of the course date
  • leave ORA dates alone so they are only adjusted manually

My understanding is that this PR's functionality would essentially mirror the first option listed above. Please let me know if you think I'm missing something here and happy to keep talking it through.

@jansenk I know you were going to look into this a little more after our chat yesterday to so feel free to amend anything I'm saying here. thanks

CC: @e0d

@Ian2012
Copy link
Author

Ian2012 commented Aug 16, 2023

Basically, it is. But also, allow instructors to extend a student's due date based on individual due date extensions.

As it aligns with your use case. Could you provide further instructions for this PR? @Daniel-hershel

@Daniel-hershel
Copy link

@Ian2012 if you think there is still additional value in this PR that isn't represented in the work Jansen is doing, can you please help me understand what that additional value is?

The other question is whether anything in this PR will conflict with the release we are preparing to make. That is something that @jansenk would have to weigh in on.

Thanks

@mariajgrimaldi
Copy link
Member

@Daniel-hershel: if the functionality you folks have been working on includes the use case of allow instructors to extend a student's due date based on individual due date extensions, then I think we're good to go since it's basically what we're attempting here. Should we close this PR then? Also, let us know when you release the new version so we can test it!

@Daniel-hershel
Copy link

@jansenk can you please confirm the answer to the question @mariajgrimaldi is asking. Thanks!

@Ian2012 Ian2012 closed this Aug 29, 2023
@openedx-webhooks
Copy link

@Ian2012 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@Ian2012
Copy link
Author

Ian2012 commented Aug 29, 2023

Based on this discussion #1959 (comment) this PR will be closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Allow individual learner extension for ORA responses
7 participants